Skip to content

Linear algebra support + stricter name/arg validation - #87

Merged
t-kalinowski merged 28 commits into
mainfrom
la-updates
Jan 28, 2026
Merged

Linear algebra support + stricter name/arg validation#87
t-kalinowski merged 28 commits into
mainfrom
la-updates

Conversation

@t-kalinowski

Copy link
Copy Markdown
Owner

Motivation

This branch expands quickr’s ability to transpile “plain R” linear algebra code (the kind you’d typically write first, then optimize) while tightening correctness around symbol/argument handling. The intent is to make porting base-R numeric code to quick() smoother, and to surface mismatches early with clear errors.

User-facing changes

  • Linear algebra: additional base-R compatibility for common patterns
    • drop() for rank 0–2 inputs (including singleton matrices).
    • svd() results usable via $d, $u, and $v (either s <- svd(x); s$d or svd(x)$d).
    • .Machine$double.eps is supported.
    • qr.solve() uses the LINPACK QR path for improved compatibility with base R.
  • Names and arguments
    • Dotted symbols are supported in quick() (arguments, locals, loop variables), e.g. foo.bar.
    • Conflicting names that map to the same Fortran symbol now error (Fortran is case-insensitive).
    • Using a function argument without declare(type(arg = ...)) now errors.
  • Semantics/correctness
    • Vector–matrix recycling in arithmetic is restricted to nrow to match base R.
    • Local closures now support optional arguments with NULL defaults, with validation that optional arguments are initialized (via an is.null() branch) before use.
  • Fixes
    • Fixes a case where the generated C bridge could reference hoisted size expressions before they were emitted (compilation failure).

Examples

svd() + .Machine$double.eps + qr.solve():

q_fast_lm <- quick(function(X, y) {
  declare(type(X = double(n, k)), type(y = double(n)))

  coef <- qr.solve(X, y)

  XtX <- crossprod(X)
  s <- svd(XtX)
  tol <- max(dim(X)) * max(s$d) * .Machine$double.eps

  coef
})

Optional NULL arguments in local closures (must initialize before use):

q <- quick(function(x) {
  declare(type(x = double(n)))

  f <- function(i, offset = NULL) {
    if (is.null(offset)) offset <- 1
    x[i] + offset
  }

  sapply(seq_along(x), f)
})

Release notes

NEWS.md is updated to reflect these user-facing changes.

- Add min()/max() support in size expressions
- Hoist/reuse Rf_asInteger() results to avoid repeated conversions in C
- Add public-API tests via quick() and update translation snapshots
- Reject non-scalar vectors unless length equals matrix nrow
- Add/adjust elementwise matrix tests for the stricter rule
- Refresh translation snapshots
- Detect use of formal args without declare()
- Add a public-API regression test
- Reject names that map to reserved identifiers
- Snapshot full error messages for name clashes
- Preserve R names while fortranizing identifiers
- Fix C bridge / closure codegen name resolution
- Add public API tests for dotted args, locals, and loops
- Translate to Fortran epsilon()
- Add quick() test and translation snapshot
- Compile svd() via LAPACK dgesdd()
- Allow assigning svd() results and extracting components
- Add quick() tests and translation snapshot
- Translate known singleton dims to slices/scalars
- Add quick() tests and translation snapshots (NA/n dims)
- detect unguarded optional arg usage in closures
- add regression test for NULL default without init
@codecov

codecov Bot commented Jan 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.70000% with 93 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.76%. Comparing base (fa7ada1) to head (9e017d9).
⚠️ Report is 29 commits behind head on main.

Files with missing lines Patch % Lines
R/r2f-closures.R 85.43% 52 Missing ⚠️
R/c-wrapper.R 88.47% 25 Missing ⚠️
R/r2f-aab-core.R 80.00% 4 Missing ⚠️
R/r2f-matrix-blas.R 97.29% 4 Missing ⚠️
R/scope.R 90.62% 3 Missing ⚠️
R/aaa-utils.R 90.90% 2 Missing ⚠️
R/r2f-matrix.R 98.55% 1 Missing ⚠️
R/r2f-operators-helpers.R 98.33% 1 Missing ⚠️
R/sizes.R 93.75% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #87      +/-   ##
==========================================
- Coverage   92.79%   92.76%   -0.04%     
==========================================
  Files          27       27              
  Lines        4888     5734     +846     
==========================================
+ Hits         4536     5319     +783     
- Misses        352      415      +63     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 89edddee36

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread R/sizes.R
Comment on lines 163 to 165
if (var@is_arg && !var@modified) {
return(r)
return(scope_fortran_symbol(r, scope))
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep R-size symbols resolvable for dotted args

When r2size() sees an argument symbol it now returns scope_fortran_symbol() (e.g., foo.bar becomes foo_bar). That propagates into dimension expressions used by constructors/locals (e.g., out <- double(foo.bar)), so the return variable carries a size symbol foo_bar. The C bridge (dims2c_expr in c-wrapper.R) still resolves size symbols via get0() on the R name, so it can’t resolve foo_bar and will error (“could not resolve size: foo_bar”) or emit invalid C. This breaks compiled functions that size outputs from dotted arguments; either keep R symbols in r2size() or teach dims2c_expr/size resolution to map Fortranized names back to the scoped variable.

Useful? React with 👍 / 👎.

@t-kalinowski

Copy link
Copy Markdown
Owner Author

Pushed commit 3b7d555 adding public-API tests (all via quick() and exercising the compiled call path) to address Codecov patch-coverage gaps.

New tests cover:

  • declare() size-expression support for abs(), nrow(), ncol(), and dim(x)[axis].
  • Local-closure optional NULL defaults (including multiple optional args), argument validation paths, and the error when NULL-returning closures are used as values.
  • Vector–matrix elementwise recycling restricted to nrow.

Also extended dims2f to recognize nrow()/ncol()/dim(x)[axis] in declared sizes so the new tests compile (matching the existing dims2c support).

@t-kalinowski

Copy link
Copy Markdown
Owner Author

Addressed the review note about dotted args in size expressions.

  • Fix: R/manifest.R now excludes Fortranized formal arg names from the sizes block, avoiding duplicate declarations when a dotted formal (e.g. foo.barfoo_bar) is used as an array extent.
  • Fix: R/c-wrapper.R now resolves size-expression symbols by either R name or Fortran name (scope_var_by_fortran_name()), so foo_bar in extents maps back to the formal.
  • Regression: tests/testthat/test-dotted-arg-size-expr.R compiles via quick() and calls the compiled function.

Pushed in commit f67fd29.

@t-kalinowski
t-kalinowski merged commit f22ae80 into main Jan 28, 2026
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant